Skip to content

Conversation

will-v-pi
Copy link
Contributor

Prevent printing metadata block twice in picotool info, when only one block is present in the block loop

Noticed in #270

Before:

Metadata Block 1
 address:                0x10000014
 next block address:     0x10000014
 block type:             image def
 target chip:            RP2350
 image type:             RISC-V
 entry point:            EP 0x10000036, SP 0x20082000

Metadata Block 2
 address:                0x10000014
 next block address:     0x10000014
 block type:             image def
 target chip:            RP2350
 image type:             RISC-V
 entry point:            EP 0x10000036, SP 0x20082000

After:

Metadata Block 1
 address:             0x10000014
 next block address:  0x10000014
 block type:          image def
 target chip:         RP2350
 image type:          RISC-V
 entry point:         EP 0x10000036, SP 0x20082000

select_group(metadata_info[block_i++], true);
info_metadata(block.get(), true);
if (first_block->next_block_rel != 0) { // if there is more than one block
auto all_blocks = get_all_blocks(bin, raw_access.get_binary_start(), first_block, more_cb);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, does this change imply that if there are two or more blocks, then get_all_blocks doesn't return the first block; but if there's only one block, then get_all_blocks does return the first block??
Or am I just mis-reading the diff?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, get_all_blocks returns all other blocks when there are multiple blocks, but it returns the first block again if there is only one block

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a stupid question (I'm good a those 😆 ), but I wonder if that suggests that get_all_blocks should instead return zero blocks if there is only one block? Seems like it might be more consistent behaviour? 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's probably for another PR, as I'd need to fix some places it's used so that behaviour works

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough; not being familiar with the code, I didn't realise that you were relying on the existing behaviour in other places.

@lurch
Copy link
Contributor

lurch commented Aug 12, 2025

Maybe it's worth adding a regression-test for this to CI? 🤔

@will-v-pi
Copy link
Contributor Author

Maybe it's worth adding a regression-test for this to CI? 🤔

That'd probably be a separate PR, to add regression testing to the info output (lots of other bits to check too, eg the binary info)

@will-v-pi will-v-pi merged commit e10952c into develop Aug 13, 2025
44 checks passed
@will-v-pi will-v-pi deleted the duplicate-metadata branch August 13, 2025 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants